cargo xtask validate just takes too long on my machine, aborted after 10 minutes of execution. I have run cargo test -p cubecl-wgpu and cargo test -p cubecl-core instead and they seem to work.

Not sure if it might be a good idea to pull all the trigonometry functions (including the existing ones) into a new enum for the IR, because that's a lot of added stuff, and it's now enough to warrant its own category I think. We should at some point also think about applying a similar separation to the compilers themselves, but that's a larger rework that wouldn't go into this PR.

I'm currently looking into the MLIR functions for that stuff - the bindings are auto-generated I believe, so if the functions don't exist, they might be in a different namespace or need a different way to handle them.

As for the float types, casting to float for unsupported float types (i.e. F16, BF16) is reasonable. There do appear to be double versions of the functions, so that will work natively.

@wingertge

Not sure if it might be a good idea to pull all the trigonometry functions (including the existing ones) into a new enum for the IR, because that's a lot of added stuff, and it's now enough to warrant its own category I think.

Must I implement this in this PR or will this be part of another refactor?

As for the float types, casting to float for unsupported float types (i.e. F16, BF16) is reasonable. There do appear to be double versions of the functions, so that will work natively.

Can you please double-check my implementation? I don't really understand at which point the right functions are written to the shader for each dialect...

@wingertge

Not sure if it might be a good idea to pull all the trigonometry functions (including the existing ones) into a new enum for the IR, because that's a lot of added stuff, and it's now enough to warrant its own category I think.

Must I implement this in this PR or will this be part of another refactor?

I think that would be better to do after all the outstanding work has been merged, so I'll do it separately.
One more thing I'd add retroactively is that since there's no intrinsics for converting degrees/radians, I think it would be better to just implement those as cube functions in cubecl-std, rather than the compiler. The implementation would be trivial:

#[cube]
pub fn to_degrees<F: Float>(val: F) -> F {
    val * F::new(180.0 / f32::PI)
}

It would keep it in one place rather than implemented separately for each compiler.

Can you please double-check my implementation? I don't really understand at which point the right functions are written to the shader for each dialect...

CUDA is already correct, not sure what the WGSL behaviour even is because it's an experimental extension. It might just support f16 overloads on those functions by default, but I can't yet test it because of some issues with features on Vulkan.
For SPIR-V I think it would be best implemented by a preprocessor but they're poorly documented, so I think the easiest way to do it is to open an issue to track that it still needs to be done and I'll deal with it myself.

there's no intrinsics for converting degrees/radians

There are intrinsics for WGSL and SPIR-V (at least rspirv-ext), also Rusts f32 and f64 do have to_degrees and to_radians operations. Also, as a user, I would probably search in the Float Namespace for these functions instead of the cubecl-std. That's why I think having them there would make still sense.

I removed the dialect specific compile_instruction_xxx_scalar calls and instead hardcode the operations directly for the cubecl-cpp, since all CUDA, HIP and Metal should convert implicit to the right type and share the same syntax.
I also added the to_degrees and to_radians functions to the cubecl-std, so that you can decide which one fits best / should survive a refactor.

Before things are getting stale, I mark this as ready to review.

Currently, 21 tests are failing, 20 of them related to the missing CPU implementation of the inverse trigonometric functions (arc-*) and one is that to_degrees fails for f16:

Values differ more than epsilon: actual=89.9375, expected=90, difference=0.0625, epsilon=0.020004272
index: 1
actual: [0.0, 89.9375, 179.875, -89.9375, -179.875]
expected: [0.0, 90.0, 180.0, -90.0, -180.0]

For both problems I can't do more, since the first one needs to be fixed by @marcantoinem I guess? and the second one needs to be fixed by a design decision whether to_degrees (and to_radians) should be present for Float types or not and if yes, whether it should be okay to lower the test-epsilons for this.

If I should do more, let me know.

Currently, 21 tests are failing, 20 of them related to the missing CPU implementation of the inverse trigonometric functions (arc-*) and one is that to_degrees fails for f16:

that epsilon looks too tight for f16, might have another bug in the precision-aware comparison like when I forgot an abs and made the tolerance way too tight for negative values. This one is positive, but seems excessively precise regardless so something needs to be adjusted there. Maybe we can modify all tests to specify epsilon, since it can depend on the exact operation.

The code looks fine to me, but the tests don't pass on the CI. @relativityhd We may also remove some functions in the trigo modules, unsure they are necessary.

@nathanielsimard yes the CI is failing because as I mentioned this PR is blocked by the MLIR melior project, because the math module there wont register correctly, as @marcantoinem mentioned.
The tests are failing because of the empty implementations of the functions depending on the melior llvm math module.

We may also remove some functions in the trigo modules, unsure they are necessary.

Sure, just tell which ones I should remove.

Sure, just tell which ones I should remove.

The ones that are not stricly necessary, using if statements is also very bad on most hardware, so we can remove the functions that have if statements.

Sorry for the long wait - my vacation is over and time limited again...

I've removed unnecessary trig functions from the std, only keeping hypot, to_radians and to_degrees since they are also present in rusts f32.

This PR is still blocked by the MLIR melior project. I still get the following error when testing (I've removed the dummy code and enabled ods math support again, as @marcantoinem described):

error: cannot be converted to LLVM IR: missing `LLVMTranslationDialectInterface` registration for dialect for op: math.acos

@nathanielsimard What repository is responsible for this? Maybe I can check and fix it there. I am quite confused about the different LLVM repositories and packages used.

@nathanielsimard What repository is responsible for this? Maybe I can check and fix it there. I am quite confused about the different LLVM repositories and packages used.

https://github.com/tracel-ai/tracel-llvm :)

So I checked whether I would find any reason why the math dialect is not properly registered, but I just can't find it. Seems to be present everywhere where other dialects are also present, e.g. Arith.
But I am also completely foreign to LLVM and compilers in general, so I think someone of your group should take a look here.
@marcantoinem can you maybe take a look and find out what is missing?

I have added you all to my repo in case I am blocking your development, just in case

I have merged from main, but still no success. Still got the error: cannot be converted to LLVM IR: missing LLVMTranslationDialectInterface registration for dialect for op: func.func when running cargo xtask validate...

I've tried running with the debug flag you mentioned CUBECL_MLIR_DEBUG="./debug-mlir/" cargo xtask validate but I can't see a new directory anywhere...

You have to activate feature mlir-dump, for example:

CUBECL_DEBUG_MLIR=/tmp/mlir2 cargo test --features mlir-dump -p cubecl-cpu tests::f32_ty::unary:: -- --nocapture

The order of the registration of the conversion of the dialects is important, the above commit should fix it.

OMG yes it finally works, thank you!

So I guess this PR is now ready for merge! @wingertge @nathanielsimard

Sooo, tests failing on my machine, but the functions which are failing are outside this PR (plane-shuffle function). From my end everything should be fine. 👍🏻